fix: resolve EXC_BAD_ACCESS in SentryTracer/SentryNetworkTracker span lifecycle - #8058
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8058 +/- ##
=============================================
- Coverage 87.306% 87.203% -0.104%
=============================================
Files 554 559 +5
Lines 31986 32172 +186
Branches 13139 13146 +7
=============================================
+ Hits 27926 28055 +129
- Misses 4012 4069 +57
Partials 48 48
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
… lifecycle Fix two crash paths in the SentryTracer → SentryNetworkTracker call chain that cause use-after-free crashes (SDK-CRASHES-COCOA-5, ~140K events/90d): 1. TOCTOU race in canBeFinished: hasUnfinishedChildSpansToWaitFor was checked outside @synchronized(self), allowing concurrent threads to both see "no unfinished children" and call finishInternal. Moved the check inside the synchronized block. 2. Volatile currentRequest re-reads: SentryTracePropagation and SentryNetworkTracker accessed sessionTask.currentRequest multiple times without retaining. The property can return a freed object if the task completes on another thread between reads. Snapshot the request once into a local variable in both methods. 3. Weak span dangling refs in TTD callback: SentryTimeToDisplayTracker's finishCallback accessed weak initialDisplaySpan/fullDisplaySpan properties repeatedly. Strongify into locals at callback entry with nil guards. Closes #8012
Cover the three crash paths fixed in the previous commit: - SentryTracerTests: concurrent child span finish racing with tracer.finish() to verify canBeFinished atomicity - SentryNetworkTrackerTests: concurrent resume + setState on the same task, and resume after task already completed - SentryTimeToDisplayTrackerTest: concurrent tracer finish with child span operations, and finish with no full display span - SentryTracePropagationTests: addBaggageHeader with nil currentRequest (task with no request set)
ad1d81a to
2712c25
Compare
…acer/SentryNetworkTracker span lifecycle
📲 Install BuildsiOS
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bbc91e5 | 1236.06 ms | 1264.39 ms | 28.32 ms |
| 207a888 | 1215.22 ms | 1248.98 ms | 33.75 ms |
| 3efa7b5 | 1226.55 ms | 1260.66 ms | 34.11 ms |
| 71859d3 | 1216.45 ms | 1259.12 ms | 42.67 ms |
| f82ca9b | 1227.06 ms | 1264.67 ms | 37.61 ms |
| e983818 | 1231.33 ms | 1265.04 ms | 33.72 ms |
| 1770336 | 1225.09 ms | 1251.32 ms | 26.23 ms |
| 4d144d7 | 1225.77 ms | 1255.85 ms | 30.09 ms |
| af5922b | 1224.16 ms | 1250.40 ms | 26.24 ms |
| 9787f05 | 1233.67 ms | 1269.00 ms | 35.33 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bbc91e5 | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| 207a888 | 24.14 KiB | 1.18 MiB | 1.15 MiB |
| 3efa7b5 | 24.14 KiB | 1.22 MiB | 1.19 MiB |
| 71859d3 | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| f82ca9b | 24.14 KiB | 1.17 MiB | 1.14 MiB |
| e983818 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| 1770336 | 24.14 KiB | 1.15 MiB | 1.13 MiB |
| 4d144d7 | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| af5922b | 24.14 KiB | 1.17 MiB | 1.15 MiB |
| 9787f05 | 24.14 KiB | 1.18 MiB | 1.15 MiB |
Previous results on branch: fix/exc-bad-access-tracer-network-span-lifecycle
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 74e496b | 1229.24 ms | 1255.89 ms | 26.65 ms |
| a6c6416 | 1220.39 ms | 1243.80 ms | 23.41 ms |
| aed83c2 | 1232.90 ms | 1262.83 ms | 29.93 ms |
| 6fdbc97 | 1212.43 ms | 1248.49 ms | 36.06 ms |
| 1119cfd | 1232.30 ms | 1269.40 ms | 37.10 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 74e496b | 24.14 KiB | 1.23 MiB | 1.21 MiB |
| a6c6416 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| aed83c2 | 24.14 KiB | 1.23 MiB | 1.20 MiB |
| 6fdbc97 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| 1119cfd | 24.14 KiB | 1.22 MiB | 1.20 MiB |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Moving hasUnfinishedChildSpansToWaitFor inside @synchronized(self) introduced an ABBA deadlock: canBeFinished takes self then _children, while finishTracer holds _children and re-enters canBeFinished via spanFinished, waiting on self. The original TOCTOU was already guarded by the double-check lock in finishTracer (lines 580-588), so the move was unnecessary.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…to ensure timestamp assignment
I believe this is one of the limitation of swizzling native frameworks, we don't know how apple may access / modify the tasks so we cant do much aside from protecting every time we access the object. |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
LGTM, with my take (partly worked out with Claude Code) — this improves things, so 👍 to merge as-is.
Recording my assumptions here; whether to chase the root cause / open an issue is up to you @itaybre.
My main focus is the swizzle in SentrySwizzleWrapperHelper.m#L101-L113: our tracker logic runs before SentrySWCallOriginal(). The original NSURLSessionTask code is presumably where the thread-safety (locking) lives, so by running first we execute outside that protection and then hand control to the original.
The reason this matters: NSURLSessionTask being "thread-safe" mostly means you can drive it (resume/cancel) from any thread and results are delivered at quiescent points on the delegate queue — not that property reads are atomic while a request is in flight. A normal caller only touches the task at those quiescent points, so it never races. We do the opposite: we read task state, and we add trace/baggage headers from various threads (SentryTracePropagation.m#L44-L59), at moments URLSession may be mutating the task concurrently. The reported CFURLRequestSetHTTPRequestBody crash lines up with a freed currentRequest being copied there.
So this PR narrows the race window but I don't think it removes it — the remaining access is still unsynchronized. That's fine for a mitigation; I just wouldn't call it a root-cause fix.
Longer term, it looks like properly fixing this would take some dedicated time, and it could turn out we need to rethink how our whole swizzling approach here works — potentially a bigger project. I completely understand if we don't want to touch that right now.
…uest for better stability
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a6895e. Configure here.
Remove unused helper that still performed volatile double-reads of currentRequest. Update changelog to accurately describe the fix.
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
3 participants
Description
Fix EXC_BAD_ACCESS crashes in the
SentryNetworkTrackercall chain caused by repeated reads of the volatileNSURLSessionTask.currentRequestproperty. This is the most frequent crash pattern in SDK-CRASHES-COCOA-5 (43/100 sampled events), affecting 7 customer projects across SDK versions 8.36.0–9.9.0 with ~140K events in the last 90 days.Root cause
NSURLSessionTask.currentRequestis acopyproperty managed by the URL loading system. When a task completes or redirects on another thread, repeated reads can return a freed object. Since we don't own the task (it's managed by the user's code and the OS), we can't synchronize access — the only safe approach is to snapshot the property once into a local variable at each entry point.Fixes
1. Snapshot
currentRequestinurlSessionTaskResume:— the property was read 3 times (URL, HTTPMethod for span description, HTTPMethod for data). Now snapshotted once into a local variable.2. Snapshot
currentRequestinurlSessionTask:setState:— the property was read multiple times acrosscaptureFailedRequests:(3 reads) andaddBreadcrumbForSessionTask:(2 reads). Now snapshotted once at the entry point and threaded through as a parameter.3. Snapshot
currentRequestinaddBaggageHeader:— the property was read multiple times for URL matching, header reading, and request mutation. Now snapshotted once.4. Defensive strongification in TTD callback —
SentryTimeToDisplayTracker'sfinishCallbackstrongifies weakinitialDisplaySpan/fullDisplaySpanreferences into locals at callback entry with nil guards. This is a defensive hardening (the tracer holds children strongly during the callback, so the race is theoretical).5. Cleanup — removed dead
sessionTaskRequiresPropagation:method.How tested
make format— cleanmake analyze— cleanmake build-ios— succeedsVolatileRequestTaskMock— a mock wherecurrentRequestreturns nil after N accesses, deterministically simulating the production racetestSpanData_VolatileCurrentRequest_UsesSnapshot— fails without the fix ("(null) https://...", nil method), passes with itmake test-ios ONLY_TESTING=SentryTests/SentryNetworkTrackerTests— 76 tests passmake test-ios ONLY_TESTING=SentryTests/SentryTracePropagationTests— 10 tests passmake test-ios ONLY_TESTING=SentryTests/SentryTracerTests— 88 tests passmake test-ios ONLY_TESTING=SentryTests/SentryTimeToDisplayTrackerTest— 19 tests passCloses #8012